<!DOCTYPE html>
<html lang="en" itemscope itemtype="https://schema.org/WebPage">
  <head>
    <meta charset="utf-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <title>Vitess / Code Reviews</title>

        <!-- Webfont -->
    <link href='http://fonts.googleapis.com/css?family=Roboto:400,100,100italic,300,300italic,400italic,500,500italic,700,700italic,900,900italic' rel='stylesheet' type='text/css'>
    
    <!--<link rel="shortcut icon" type="image/png" href="/favicon.png">-->

    <!-- Bootstrap -->
    <link href="/libs/bootstrap/css/bootstrap.min.css" rel="stylesheet">

    <!-- HTML5 shim and Respond.js for IE8 support of HTML5 elements and media queries -->
    <!-- WARNING: Respond.js doesn't work if you view the page via file:// -->
    <!--[if lt IE 9]>
      <script src="https://oss.maxcdn.com/html5shiv/3.7.2/html5shiv.min.js"></script>
      <script src="https://oss.maxcdn.com/respond/1.4.2/respond.min.js"></script>
    <![endif]-->

    <!-- Styles -->
    <link rel="stylesheet" type="text/css" href="/css/site.css" />
    <!-- Font Awesome icons -->
    <link href="/libs/font-awesome-4.4.0/css/font-awesome.min.css"
          rel="stylesheet"
          type="text/css">
    
    <!-- jQuery (necessary for Bootstrap's JavaScript plugins) -->
    <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.2/jquery.min.js"></script>
    <!-- Include all compiled plugins (below), or include individual files as needed -->
    <script src="/libs/bootstrap/js/bootstrap.min.js"></script>


    <!-- metadata -->
    <meta name="og:title" content="Vitess / Code Reviews"/>
    <meta name="og:image" content="/images/vitess_logo_with_border.svg"/>
    <meta name="og:description" content="Vitess is a database clustering system for horizontal scaling of MySQL."/>

    <link rel="icon" href="/images/vitess_logo_icon_size.png" type="image/png">

    
    <script>
  (function(i,s,o,g,r,a,m){i['GoogleAnalyticsObject']=r;i[r]=i[r]||function(){
  (i[r].q=i[r].q||[]).push(arguments)},i[r].l=1*new Date();a=s.createElement(o),
  m=s.getElementsByTagName(o)[0];a.async=1;a.src=g;m.parentNode.insertBefore(a,m)
  })(window,document,'script','//www.google-analytics.com/analytics.js','ga');
  ga('create', 'UA-60219601-1', 'auto');
  ga('send', 'pageview');
</script>

    
  </head>
  <body class="docs" id="top_of_page">

    <span id="toc-depth" data-toc-depth="h2,h3"></span>


    <nav id="common-nav" class="navbar navbar-fixed-top inverse">
  <div class="container">
    <!-- Brand and toggle get grouped for better mobile display -->
    <div class="navbar-header">
      <button type="button" class="navbar-toggle collapsed" data-toggle="collapse" data-target="#navbar-collapse-1">
        <span class="sr-only">Toggle navigation</span>
        <span class="icon-bar"></span>
        <span class="icon-bar"></span>
        <span class="icon-bar"></span>
      </button>
      <a class="navbar-brand" href="/">
        <img class="logo" src="/images/vitess_logo_with_border.svg" alt="Vitess">
      </a>
    </div>

    <!-- Collect the nav links, forms, and other content for toggling -->
    <div class="collapse navbar-collapse" id="navbar-collapse-1">
      <ul class="nav navbar-nav navbar-right" id="standard-menu-links">
        <li><a href="/overview/">Overview</a></li>
        <li><a href="/user-guide/introduction.html">Guides</a></li>
        <li><a href="/reference/vitess-api.html">Reference</a></li>
        <li><a href="http://blog.vitess.io">Blog</a></li>
        <li><a href="https://github.com/youtube/vitess"><i class="fa fa-github"></i> GitHub</a></li>
        <!-- Hide link to blog unless we have actual posts -->
        <!-- <li><a href="/blog/" title="">Blog</a></li> -->
      </ul>
      <ul class="nav nav-stacked mobile-left-nav-menu" id="collapsed-left-menu">
                <li class="submenu">
          <h4 class="arrow-r no-top-margin">Overview</h4>
          <ul style="display: none">
            <li><a href="/overview/">What is Vitess</a></li>
            <li><a href="/overview/scaling-mysql.html">Scaling MySQL with Vitess</a></li>
            <li><a href="/overview/concepts.html">Key Concepts</a></li>
          </ul>
        </li>
        <li class="submenu">
          <h4 class="arrow-r">Getting Started</h4>
          <ul style="display: none">
            <li style="padding-bottom: 0"><a href="/getting-started/">Run Vitess on Kubernetes</a>
              <ul style="display: block">
                <li style="padding-bottom: 0"><a href="/getting-started/docker-build.html">Custom Docker Build</a></li>
              </ul>
            </li>
            <li><a href="/getting-started/local-instance.html">Run Vitess Locally</a></li>
          </ul>
        </li>
        <li class="submenu">
          <h4 class="arrow-r">User Guide</h4>
          <ul style="display: none">
            <li><a href="/user-guide/introduction.html">Introduction</a>
            <li><a href="/user-guide/client-libraries.html">Client Libraries</a>
            <li><a href="/user-guide/backup-and-restore.html">Backing Up Data</a>
            <li><a href="/user-guide/reparenting.html">Reparenting</a></li>
            <li style="padding-bottom: 0"><a href="/user-guide/schema-management.html">Schema Management</a></li>
              <ul style="display: block">
                <li style="padding-bottom: 0"><a href="/user-guide/vschema.html">VSchema Guide</a></li>
                <li style="padding-bottom: 0"><a href="/user-guide/schema-swap.html">Schema Swap (Tutorial)</a></li>
              </ul>
            <li style="padding-bottom: 0"><a href="/user-guide/sharding.html">Sharding</a>
              <ul style="display: block">
                <li><a href="/user-guide/horizontal-sharding-workflow.html">Horizontal Sharding (Tutorial, automated)</a></li>
                <li><a href="/user-guide/horizontal-sharding.html">Horizontal Sharding (Tutorial, manual)</a></li>
                <li><a href="/user-guide/sharding-kubernetes-workflow.html">Sharding in Kubernetes (Tutorial, automated)</a></li>
                <li style="padding-bottom: 0"><a href="/user-guide/sharding-kubernetes.html">Sharding in Kubernetes (Tutorial, manual)</a></li>
              </ul>
            </li>
            <li><a href="/user-guide/vitess-sequences.html">Vitess Sequences</a></li>
            <li><a href="/user-guide/mysql-server-protocol.html">MySQL Server Protocol</a></li>
            <li><a href="/user-guide/vitess-replication.html">Vitess and Replication</a></li>
            <li><a href="/user-guide/update-stream.html">Update Stream</a></li>
            <li><a href="/user-guide/row-based-replication.html">Row Based Replication</a></li>
            <li><a href="/user-guide/topology-service.html">Topology Service</a></li>
            <li><a href="/user-guide/transport-security-model.html">Transport Security Model</a></li>
            <li style="padding-bottom: 0"><a href="/user-guide/launching.html">Launching</a>
              <ul style="display: block">
                <li><a href="/user-guide/scalability-philosophy.html">Scalability Philosophy</a></li>
                <li><a href="/user-guide/production-planning.html">Production Planning</a></li>
                <li><a href="/user-guide/server-configuration.html">Server Configuration</a></li>
                <li><a href="/user-guide/twopc.html">2PC Guide</a></li>
                <li style="padding-bottom: 0"><a href="/user-guide/troubleshooting.html">Troubleshooting</a></li>
              </ul>
            </li>
            <li><a href="/user-guide/upgrading.html">Upgrading</a></li>
          </ul>
        </li>
        <li class="submenu">
          <h4 class="arrow-r">Reference Guides</h4>
          <ul style="display: none">
            <li><a href="/reference/vitess-api.html">Vitess API</a>
            <li><a href="/reference/vtctl.html">vtctl Commands</a>
          </ul>
        </li>
        <li class="submenu">
          <h4 class="arrow-r">Other Resources</h4>
          <ul style="display: none">
            <li><a href="/resources/presentations.html">Presentations</a>
            <li><a href="http://blog.vitess.io/">Blog</a>
            <li><a href="/resources/roadmap.html">Roadmap</a>
          </ul>
        </li>
        <li class="submenu">
          <h4 class="arrow-r">Contributing</h4>
          <ul style="display: none">
            <li><a href="/contributing/">Contributing to Vitess</a>
            <li><a href="/contributing/github-workflow.html">GitHub Workflow</a>
            <li><a href="/contributing/code-reviews.html">Code Reviews</a>
          </ul>
        </li>
        <li class="submenu">
          <h4 class="arrow-r">Internal</h4>
          <ul style="display: none">
            <li><a href="/internal/">Overview</a>
            <li><a href="/internal/release-instructions.html">Release Instructions</a>
            <li><a href="/internal/publish-website.html">Publish Website</a>
          </ul>
        </li>

        <div>
          <form method="get" action="/search/">
            <input id="search-form" name="q" type="text" placeholder="Search">
          </form>
        </div>

        <li><a href="https://github.com/youtube/vitess" id="collapsed-left-menu-repo-link"><i class="fa fa-github"></i> GitHub</a></li>
      </ul>
    </div><!-- /.navbar-collapse -->
  </div><!-- /.container-fluid -->
</nav>

    
<div id="masthead">
  <div class="container">
    <div class="row">
      <div class="col-md-9">
        <h1>Code Reviews</h1>
      </div>
    </div>
  </div>
</div>


<div class="container">
    <div class="row scroll-margin" id="toc-content-row">
    <div class="col-md-2" id="leftCol">
      <ul class="nav nav-stacked mobile-left-nav-menu" id="sidebar">
                <li class="submenu">
          <h4 class="arrow-r no-top-margin">Overview</h4>
          <ul style="display: none">
            <li><a href="/overview/">What is Vitess</a></li>
            <li><a href="/overview/scaling-mysql.html">Scaling MySQL with Vitess</a></li>
            <li><a href="/overview/concepts.html">Key Concepts</a></li>
          </ul>
        </li>
        <li class="submenu">
          <h4 class="arrow-r">Getting Started</h4>
          <ul style="display: none">
            <li style="padding-bottom: 0"><a href="/getting-started/">Run Vitess on Kubernetes</a>
              <ul style="display: block">
                <li style="padding-bottom: 0"><a href="/getting-started/docker-build.html">Custom Docker Build</a></li>
              </ul>
            </li>
            <li><a href="/getting-started/local-instance.html">Run Vitess Locally</a></li>
          </ul>
        </li>
        <li class="submenu">
          <h4 class="arrow-r">User Guide</h4>
          <ul style="display: none">
            <li><a href="/user-guide/introduction.html">Introduction</a>
            <li><a href="/user-guide/client-libraries.html">Client Libraries</a>
            <li><a href="/user-guide/backup-and-restore.html">Backing Up Data</a>
            <li><a href="/user-guide/reparenting.html">Reparenting</a></li>
            <li style="padding-bottom: 0"><a href="/user-guide/schema-management.html">Schema Management</a></li>
              <ul style="display: block">
                <li style="padding-bottom: 0"><a href="/user-guide/vschema.html">VSchema Guide</a></li>
                <li style="padding-bottom: 0"><a href="/user-guide/schema-swap.html">Schema Swap (Tutorial)</a></li>
              </ul>
            <li style="padding-bottom: 0"><a href="/user-guide/sharding.html">Sharding</a>
              <ul style="display: block">
                <li><a href="/user-guide/horizontal-sharding-workflow.html">Horizontal Sharding (Tutorial, automated)</a></li>
                <li><a href="/user-guide/horizontal-sharding.html">Horizontal Sharding (Tutorial, manual)</a></li>
                <li><a href="/user-guide/sharding-kubernetes-workflow.html">Sharding in Kubernetes (Tutorial, automated)</a></li>
                <li style="padding-bottom: 0"><a href="/user-guide/sharding-kubernetes.html">Sharding in Kubernetes (Tutorial, manual)</a></li>
              </ul>
            </li>
            <li><a href="/user-guide/vitess-sequences.html">Vitess Sequences</a></li>
            <li><a href="/user-guide/mysql-server-protocol.html">MySQL Server Protocol</a></li>
            <li><a href="/user-guide/vitess-replication.html">Vitess and Replication</a></li>
            <li><a href="/user-guide/update-stream.html">Update Stream</a></li>
            <li><a href="/user-guide/row-based-replication.html">Row Based Replication</a></li>
            <li><a href="/user-guide/topology-service.html">Topology Service</a></li>
            <li><a href="/user-guide/transport-security-model.html">Transport Security Model</a></li>
            <li style="padding-bottom: 0"><a href="/user-guide/launching.html">Launching</a>
              <ul style="display: block">
                <li><a href="/user-guide/scalability-philosophy.html">Scalability Philosophy</a></li>
                <li><a href="/user-guide/production-planning.html">Production Planning</a></li>
                <li><a href="/user-guide/server-configuration.html">Server Configuration</a></li>
                <li><a href="/user-guide/twopc.html">2PC Guide</a></li>
                <li style="padding-bottom: 0"><a href="/user-guide/troubleshooting.html">Troubleshooting</a></li>
              </ul>
            </li>
            <li><a href="/user-guide/upgrading.html">Upgrading</a></li>
          </ul>
        </li>
        <li class="submenu">
          <h4 class="arrow-r">Reference Guides</h4>
          <ul style="display: none">
            <li><a href="/reference/vitess-api.html">Vitess API</a>
            <li><a href="/reference/vtctl.html">vtctl Commands</a>
          </ul>
        </li>
        <li class="submenu">
          <h4 class="arrow-r">Other Resources</h4>
          <ul style="display: none">
            <li><a href="/resources/presentations.html">Presentations</a>
            <li><a href="http://blog.vitess.io/">Blog</a>
            <li><a href="/resources/roadmap.html">Roadmap</a>
          </ul>
        </li>
        <li class="submenu">
          <h4 class="arrow-r">Contributing</h4>
          <ul style="display: none">
            <li><a href="/contributing/">Contributing to Vitess</a>
            <li><a href="/contributing/github-workflow.html">GitHub Workflow</a>
            <li><a href="/contributing/code-reviews.html">Code Reviews</a>
          </ul>
        </li>
        <li class="submenu">
          <h4 class="arrow-r">Internal</h4>
          <ul style="display: none">
            <li><a href="/internal/">Overview</a>
            <li><a href="/internal/release-instructions.html">Release Instructions</a>
            <li><a href="/internal/publish-website.html">Publish Website</a>
          </ul>
        </li>

        <div>
          <form method="get" action="/search/">
            <input id="search-form" name="q" type="text" placeholder="Search">
          </form>
        </div>

      </ul>
    </div>
    <div class="col-md-3" id="rightCol">
      <div class="nav nav-stacked" id="tocSidebar">
        <div id="toc"></div>
      </div>
    </div>
    <div class="col-md-7" id="centerCol">
      <div id="centerTextCol">
        <h1 id="code-reviews">Code Reviews</h1>

<p>Every GitHub pull request must go through a code review and get approved before it will be merged into the master branch.</p>

<h2 id="what-to-look-for-in-a-review">What to look for in a Review</h2>

<p>Both authors and reviewers need to answer these general questions:</p>

<ul>
<li>  Does this change match an existing design / bug?</li>
<li>  Is there proper unit test coverage for this change? All changes should
increase coverage. We need at least integration test coverage when unit test
coverage is not possible.</li>
<li>  Is this change going to log too much? (Error logs should only happen when
the component is in bad shape, not because of bad transient state or bad
user queries)</li>
<li>  Does this change match our coding conventions / style? Linter was run and is
happy?</li>
<li>  Does this match our current patterns? Example include RPC patterns,
Retries / Waits / Timeouts patterns using Context, ...</li>
</ul>

<p>Additionally, we recommend every author to look over your own reviews just before committing them and check if you are following the recommendations below.
We usually check these kinds of things while skimming through <code class="prettyprint">git diff --cached</code> just before committing.</p>

<ul>
<li>  Scan the diffs as if you&#39;re the reviewer.

<ul>
<li>  Look for files that shouldn&#39;t be checked in (temporary/generated files).</li>
<li>  Googlers only: Remove Google confidential info (e.g. internal URLs).</li>
<li>  Look for temporary code/comments you added while debugging.

<ul>
<li>  Example: fmt.Println(&quot;AAAAAAAAAAAAAAAAAA&quot;)</li>
</ul></li>
<li>  Look for inconsistencies in indentation.

<ul>
<li>  Use 2 spaces in everything except Go.</li>
<li>  In Go, just use goimports.</li>
</ul></li>
</ul></li>
<li><p>Commit message format:</p>

<ul>
<li><div class="highlight"><pre><code class="language-text" data-lang="text">&lt;component&gt;: This is a short description of the change.

If necessary, more sentences follow e.g. to explain the intent of the change, how it fits into the bigger picture or which implications it has (e.g. other parts in the system have to be adapted.)

Sometimes this message can also contain more material for reference e.g. benchmark numbers to justify why the change was implemented in this way.
</code></pre></div></li>
</ul></li>
<li><p>Comments</p>

<ul>
<li>  <code class="prettyprint">// Prefer complete sentences when possible.</code></li>
<li>  Leave a space after the comment marker <code class="prettyprint">//</code>.</li>
</ul></li>
</ul>

<p>During the review make sure you address all comments. Click Done (reviewable.io) or reply with &quot;Done.&quot; (GitHub Review) to mark comments as addressed. There should be 0 unresolved discussions when it&#39;s ready to merge.</p>

<h2 id="assigning-a-pull-request">Assigning a Pull Request</h2>

<p>If you want to address your review to a particular set of teammates, add them as Assignee (righthand side on the pull request).
They&#39;ll receive an email.</p>

<p>During discussions, you can also refer to somebody using the <em>@username</em> syntax and they&#39;ll receive an email as well.</p>

<p>If you want to receive notifications even when you aren&#39;t mentioned, you can go to the <a href="https://github.com/youtube/vitess">repository page</a> and click <em>Watch</em>.</p>

<h2 id="approving-a-pull-request">Approving a Pull Request</h2>

<p>As a reviewer you can approve a pull request through two ways:</p>

<ul>
<li>Approve the pull request via GitHub&#39;s new code review system</li>
<li>reply with a comment that contains the word <em>LGTM</em>  (Looks Good To Me)</li>
</ul>

<h2 id="merging-a-pull-request">Merging a Pull Request</h2>

<p>Pull requests can be merged after they were approved and the Travis tests have passed.
External contributions will be merged by a team member.
Internal team members can merge their <strong>own</strong> pull requests.</p>

<h2 id="internal-bug-numbers">Internal Bug Numbers</h2>

<p>Most of the bugs the team is working on are tracked internally.
We reference to them as <code class="prettyprint">b/########</code> or <code class="prettyprint">BUG=########</code> in commit messages and comments.
External users can ignore these.</p>

      </div>
    </div>
  </div>

</div>

    <div class="page-spacer"></div>
    <footer role="contentinfo" id="site-footer">
  <nav role="navigation" class="menu bottom-menu">
    
    <a href="https://groups.google.com/forum/#!forum/vitess" target="_blank">Contact: vitess@googlegroups.com</a>&nbsp;&nbsp;<b>·</b>&nbsp;&nbsp;
    <a href="https://groups.google.com/forum/#!forum/vitess-announce" target="_blank">Announcements</a>&nbsp;&nbsp;<b>·</b>&nbsp;&nbsp;
    &#169; 2017 <a href="/">Vitess</a> powered by <a href="https://developers.google.com/open-source/">Google Inc</a>
  </nav>
</footer>


        <!-- jQuery (necessary for Bootstrap's JavaScript plugins) -->
<!--    <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.2/jquery.min.js"></script>   -->
    <!-- Include all compiled plugins (below), or include individual files as needed -->
<!--
    <script src="/libs/bootstrap/js/bootstrap.min.js"></script>
-->
    <!-- Include the common site javascript -->
    <script src="/js/common.js" type="text/javascript" charset="utf-8"></script>


  </body>
</html>
